Skip to content

Added basic support for redis sentinel based on https://github.com/Namoshek/laravel-redis-sentinel - #184

Open
Jacq wants to merge 15 commits into
PromPHP:mainfrom
Jacq:main
Open

Jacq wants to merge 15 commits into
PromPHP:mainfrom
Jacq:main

Conversation

@Jacq

@Jacq Jacq commented Apr 12, 2025

Copy link
Copy Markdown

Merged and refactor code with from old fork #https://github.com/ml65/prometheus_client_php that supports redis sentinel by updating master address/port options before redis connections.

@LKaemmerling

Copy link
Copy Markdown
Member

Hey, thank you for your work. Would it be possible to add tests based on https://hub.docker.com/r/bitnami/redis-sentinel (see our https://github.com/PromPHP/prometheus_client_php/blob/main/.github/workflows/tests.yml)?

@Jacq

Jacq commented Apr 17, 2025

Copy link
Copy Markdown
Author

Hey, thank you for your work. Would it be possible to add tests based on https://hub.docker.com/r/bitnami/redis-sentinel (see our https://github.com/PromPHP/prometheus_client_php/blob/main/.github/workflows/tests.yml)?

Already added some test and performed a full refactor based on a different repo, using phpredis sentinel support.
Still need to fix the Redis version matrix test

@Jacq
Jacq force-pushed the main branch 2 times, most recently from e1a542d to e531f94 Compare April 18, 2025 17:19
@Jacq

Jacq commented Apr 18, 2025

Copy link
Copy Markdown
Author

Added tests, ready to merge.
It was tricky to include Redis sentinel service as redis5 does not support resolve hostname.

@Jacq Jacq changed the title Added basic support for redis sentinel Added basic support for redis sentinel based on https://github.com/Namoshek/laravel-redis-sentinel Apr 18, 2025
Brings in the 12 upstream commits since April 2025: the str_replace fix when reading metrics from Redis (PromPHP#185), Predis support (PromPHP#186) with the Redis storage split into AbstractRedis + RedisClients, PHP 8.2+ as minimum, PHP 8.5 in CI and guzzle 8 allowed.

Upstream moved the connection handling from Storage\Redis to RedisClients\PHPRedis, so the Sentinel support is re-applied there: Redis keeps the sentinel options and its public API (updateSentinelPrimary, getRedisSentinel, fromExistingConnection with an optional \RedisSentinel), and PHPRedis::ensureOpenConnection() asks Sentinel for the primary before connecting.

The reconnect loop is fixed on the way:
- it now retries: it only caught \RedisException, but getMaster() and connectToServer() already wrap it in StorageException, so no retry ever happened; the retry decision now looks at the wrapped \RedisException;
- 'reconnect' => -1 now retries indefinitely as documented; before, `while ($retries <= -1)` never ran and the client connected to the default host without asking Sentinel;
- running out of retries throws the last error instead of carrying on with no connection.

getRedisSentinel() throws StorageException when Sentinel is disabled instead of a fatal error on null.

Tests: two new Sentinel tests (reconnect -1 gets the primary; exhausting retries throws), and the misplaced @PHPStan-Ignore and trailing whitespace in RedisSentinelTest that made phpstan/phpcs fail. Full suite on PHP 8.4 against Redis 7 + Sentinel: 4211 tests OK (138 APC/APCng skipped, no apcu), phpstan and phpcs clean.

AI-Assisted-By: Claude Opus 5.5 (claude-opus-5-5, Anthropic)
AI-Oversight: merge, conflict resolution, reconnect fixes and tests generated and run by the AI; the author reviews the diff and approves the commit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants